Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

yescrypt: initial translation #509

Merged
merged 19 commits into from
Aug 18, 2024
Merged

yescrypt: initial translation #509

merged 19 commits into from
Aug 18, 2024

Conversation

tarcieri
Copy link
Member

Translation of the reference implementation as implemented in C, as of this commit:

openwall/yescrypt@e5873f8

commit e5873f89015c9c7c4ae56e3a6d17cefd47fef239
Author: Solar Designer <solar@openwall.com>
Date:   Sun Sep 24 16:00:15 2023 +0200

@tarcieri tarcieri requested a review from newpavlov July 12, 2024 04:11
@tarcieri
Copy link
Member Author

Lots of this is ugly verbatim c2rust output I'd like to clean up, but for now I tried to keep it fairly close to the reference implementation so we can ensure the KATs are passing in CI

Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This certainly needs a lot of cleanup, so I haven't checked the implementation itself.

Is there a proper specification of the algorithm? I couldn't find it after a cursory search. I am not sure it's worth to start with machine translation of the reference implementation.

Pure Rust implementation of the yescrypt password hashing function
"""
authors = ["RustCrypto Developers"]
license = "BSD-2-Clause"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think BSD-2-Clause is compatible with MIT/Apache-2.0, so we can change the license for our modification.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While they're "compatible" in that they're allowed to be used in the same program, that doesn't mean we can arbitrarily relicense code under one license under another.

We could potentially consider changing to MIT OR Apache-2.0 when the code has been extensively rewritten and is no longer an obvious derived work of the original.

Or we could attempt to get permission from the original authors to relicense it, which is what I've done (with a surprising amount of success) in the past.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... The copyright notices effectively state the same thing, but with slightly different wording. It would be great to have a "greenfield" implementation based on algorithm specification, but I don't know how hard would it be to do compared to the automatic translation (+ cleanup).

Note that right now you state MIT/Apache-2.0 licensing terms in the crate README.

yescrypt/Cargo.toml Show resolved Hide resolved
yescrypt/Cargo.toml Show resolved Hide resolved
yescrypt/tests/kats.rs Show resolved Hide resolved
yescrypt/tests/kats.rs Outdated Show resolved Hide resolved
@tarcieri tarcieri changed the title yescrypt [WIP] yescrypt Jul 12, 2024
@tarcieri
Copy link
Member Author

Probably should've noted this isn't ready for extensive comments / followup PRs yet. Just got it working. There's obviously a lot to be done.

I'd like to get 32-bit support working first, then get it merged, then do a cleanup pass PR which makes it closer to the original C source code.

@tarcieri
Copy link
Member Author

tarcieri commented Jul 12, 2024

Is there a proper specification of the algorithm? I couldn't find it after a cursory search. I am not sure it's worth to start with machine translation of the reference implementation.

I'm not sure what else to use but the reference implementation. Even as a PHC judge, I am not sure where there is a detailed description of the algorithm beyond the reference implementation, besides possibly floating around in my email history as the PHC judges list lacks a public archive.

This paper discusses some of the security properties: https://eprint.iacr.org/2015/227.pdf

This site provides a general overview, including links to slide decks which have some of the best descriptions of the algorithm I've seen: https://www.openwall.com/yescrypt/

Regardless, this is the same way the argon2 crate was made. Yes, turning the output of something like c2rust into idiomatic Rust is not fun, but it allows you to start with a working, tested implementation and iterate, and we already have PRs to swap out the vendored SHA-256/HMAC/PBKDF2 implementation for our crates: #510

@tarcieri
Copy link
Member Author

@newpavlov also, can you add github:rustcrypto:password-hashes as an owner of the yescrypt crate?

@newpavlov
Copy link
Member

Does the reference implementation confirm to this PHC submission?

can you add github:rustcrypto:password-hashes as an owner of the yescrypt crate?

Done.

@tarcieri
Copy link
Member Author

@newpavlov aha, there it is! The changelog (which is buried in a tarball, not sure if it's on GitHub somewhere) lists several changes since the PHC submission, not sure if any cause the implementation to deviate from the specification

@tarcieri tarcieri force-pushed the yescrypt branch 2 times, most recently from 33c3e5f to 2784bba Compare August 18, 2024 22:27
@tarcieri tarcieri changed the title [WIP] yescrypt yescrypt: initial translation Aug 18, 2024
@tarcieri tarcieri marked this pull request as ready for review August 18, 2024 23:06
@tarcieri tarcieri merged commit 347986e into master Aug 18, 2024
59 checks passed
@tarcieri tarcieri deleted the yescrypt branch August 18, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants